Add: export Worker from the simpler package root - #1535
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe package root now lazily exposes ChangesLazy package surface
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant simpler
participant importlib
participant simpler_worker
participant task_interface
Caller->>simpler: import simpler
Caller->>simpler: access Worker or task_interface
simpler->>importlib: import requested module
importlib->>simpler_worker: load Worker
importlib->>task_interface: load task_interface
simpler-->>Caller: return requested export
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
469b292 to
7e30c98
Compare
|
@coderabbitai review Context: the earlier attempt hit the account review limit, so this PR has not been reviewed yet. It is now rebased onto current Two things worth a close look:
|
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/ut/py/test_package_surface.py (2)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid Ruff B018 in the intentional AttributeError test.
Line 33 is deliberately evaluated, but Ruff reports it as a useless expression. Use
getattrso the test remains explicit without relying on a lint exception.Suggested fix
with pytest.raises(AttributeError, match="has no attribute"): - simpler.definitely_not_an_attribute + getattr(simpler, "definitely_not_an_attribute")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ut/py/test_package_surface.py` around lines 31 - 33, Update test_unknown_attribute_raises_attribute_error to trigger the missing attribute lookup through getattr(simpler, "definitely_not_an_attribute") instead of a standalone attribute expression, preserving the existing AttributeError assertion and message match.Source: Linters/SAST tools
19-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover
task_interfacein the advertised-surface assertion.Line 22 only checks
Workerindir(simpler), so a broken__dir__implementation that omittedtask_interfacewould still pass this test.Suggested assertion
assert "Worker" in simpler.__all__ assert "task_interface" in simpler.__all__ assert "Worker" in dir(simpler) + assert "task_interface" in dir(simpler)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ut/py/test_package_surface.py` around lines 19 - 23, Update test_worker_and_task_interface_are_advertised to also assert that "task_interface" appears in dir(simpler), covering both advertised symbols in the module’s runtime surface while preserving the existing __all__ assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/ut/py/test_package_surface.py`:
- Around line 64-70: Restrict the ImportError handling in
test_worker_resolves_to_the_same_object_as_the_submodule to skip only when the
failure indicates the _task_interface extension is unavailable. Re-raise
ImportError instances caused by other dependencies, including cloudpickle,
rather than converting them into skipped tests.
---
Nitpick comments:
In `@tests/ut/py/test_package_surface.py`:
- Around line 31-33: Update test_unknown_attribute_raises_attribute_error to
trigger the missing attribute lookup through getattr(simpler,
"definitely_not_an_attribute") instead of a standalone attribute expression,
preserving the existing AttributeError assertion and message match.
- Around line 19-23: Update test_worker_and_task_interface_are_advertised to
also assert that "task_interface" appears in dir(simpler), covering both
advertised symbols in the module’s runtime surface while preserving the existing
__all__ assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 54ccddf8-ab0f-4b10-a57e-b14274345e84
📒 Files selected for processing (5)
.claude/rules/project-layout.mddocs/user/README.mddocs/user/reference/python-api.mdpython/simpler/__init__.pytests/ut/py/test_package_surface.py
7e30c98 to
5ebab41
Compare
`simpler.__all__` advertised only the logging helpers, so the runtime's own entry point was undiscoverable: `from simpler import Worker` failed and callers had to know to write `from simpler.worker import Worker`. The package docstring already called itself the "public Python surface". `Worker` and the `task_interface` submodule now resolve through a PEP 562 module `__getattr__` rather than eager imports. Both `worker` and `task_interface` import `_task_interface` (and `worker` also `cloudpickle`) without a guard, so importing them from `__init__` would make `import simpler` fail outright whenever that extension is missing or stale — including for callers that only want the logging helpers, which `_log` deliberately keeps working through a try/except fallback. Deliberately not flattened: the task and callable types stay in `simpler.task_interface`. `simpler.task_interface.Tensor` is a nanobind device-tensor descriptor while `simpler_setup.Tensor` is a scene-test argument-spec NamedTuple — same name, unrelated types — so hoisting them into one namespace would invite exactly the confusion the export is meant to remove. project-layout.md described `simpler_setup` as internal test scaffolding. It is user-facing: examples import `KernelCompiler`, `SceneTestCase`, `Tensor`, `TaskArgsBuilder`, `ensure_pto_isa_root` and `make_tensor_arg` from it more often than they import from `simpler`, and a kernel cannot be compiled without it. The table now says so, and records the `Tensor` name collision. tests/ut/py/test_package_surface.py pins the surface: what `__all__` advertises, that unknown attributes still raise AttributeError, that `simpler.Worker` is the same object as `simpler.worker.Worker`, and the two properties the laziness exists for — `import simpler` does not pull in `simpler.worker`, and it still succeeds with no usable extension. The first of those asserts on `simpler.worker` rather than on `_task_interface`, because `_log`'s guarded probe legitimately puts the extension in `sys.modules` whenever it is importable. The identity test skips only when `ImportError.name` is `_task_interface`, and re-raises otherwise: `worker` also imports cloudpickle, and losing an unrelated dependency has to fail rather than disappear into a skipped test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai All feedback addressed and folded into the existing single commit. Inline (Major) — broad Nitpick Nitpick Verification: |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
simpler.__all__advertised only the logging helpers, so the runtime's own entry point was undiscoverable —from simpler import Workerfailed, and you had to already know to writefrom simpler.worker import Worker. Meanwhile the package docstring calls itself the "public Python surface".Workerand thetask_interfacesubmodule now resolve through a PEP 562 module__getattr__instead of eager imports.Why lazy rather than a plain import
Both
simpler/worker.py:83andsimpler/task_interface.py:35import_task_interfaceunguarded (andworkeradditionally importscloudpickle). Importing either from__init__.pywould therefore makeimport simplerfail outright whenever that extension is missing or stale — including for callers that only want the logging helpers, which_log.py:42-45deliberately keeps working through atry/exceptfallback.This is not hypothetical: on this dev box a stale
.soalready makesimport simpler.workerraisecannot import name 'TENSOR_CHILD_MEMORY_OFFSET', whileimport simplerkeeps working. Eager exports would have converted that into "the whole package is unimportable".Measured:
import simplerstays at ~0.02 s and does not loadsimpler.worker.Deliberately not flattened
The task and callable types stay in
simpler.task_interface.simpler.task_interface.Tensoris a nanobind device-tensor descriptor (Tensor.make(ptr, shape, dtype)) whilesimpler_setup.Tensoris a scene-test argument-specNamedTuple(name,value) — same name, unrelated types. Hoisting both into one namespace would invite exactly the confusion this export is meant to remove.project-layout.mdcorrectedThe rule described
simpler_setupas internal test scaffolding. It is user-facing: examples importKernelCompiler(×15),ensure_pto_isa_root(×15),make_tensor_arg(×11) and theSceneTestCasegroup (×7) from it — more often than they import fromsimpler— and a kernel cannot be compiled without it. The table now says so and records theTensorcollision.Tests
New
tests/ut/py/test_package_surface.pypins the surface: what__all__advertises, that unknown attributes still raiseAttributeError, thatsimpler.Workeris the same object assimpler.worker.Worker, and the two properties the laziness exists for —import simplerdoes not pull insimpler.worker, and it still succeeds with no usable extension.One detail worth reviewing: the first of those asserts on
simpler.worker, not on_task_interface. My initial version asserted the latter and failed — because_log's guarded probe legitimately puts the extension insys.moduleswhenever it is importable. The weaker signal would have been a false alarm forever.ruff checkandruff format --checkclean on both changed Python files.so)PYTHONPATHpython/so the complete pipeline runs